fix: detect manual changes on CRDs and CRs with three-way-merge by yxxhero · Pull Request #923 · databus23/helm-diff

yxxhero · 2026-02-01T04:19:08Z

Summary

Fixes #917

Previously, helm diff --three-way-merge would not detect manual changes made to Custom Resources (CRDs and CRs) in the cluster. This was because the code fell back to a simple JSON merge patch that only considered the old release manifest and the new chart manifest, ignoring the current live state.

For unstructured objects (CRDs, CRs), we now perform a proper three-way merge that respects manual changes made in the cluster:

Create a patch from old -> new (chart changes)
Apply this patch to current (live state with manual changes)
Create a patch from current -> merged result
Return that patch (which will be applied to current by the caller)

This ensures that manual changes to CRDs and CRs are properly detected and displayed in the diff, just like they are for built-in Kubernetes resources.

Changes

Modified manifest/generate.go to implement three-way merge for unstructured objects (CRDs, CRs)
Added detailed comments explaining the three-way merge approach
All existing tests pass

Testing

Manual testing with CRDs and CRs confirms that manual changes are now detected
All unit tests pass

Previously, helm diff --three-way-merge would not detect manual changes made to Custom Resources (CRDs and CRs) in the cluster. This was because the code fell back to a simple JSON merge patch that only considered the old release manifest and the new chart manifest, ignoring the current live state. For unstructured objects (CRDs, CRs), we now perform a proper three-way merge that respects manual changes made in the cluster: 1. Create a patch from old -> new (chart changes) 2. Apply this patch to current (live state with manual changes) 3. Create a patch from current -> merged result 4. Return that patch (which will be applied to current by the caller) This ensures that manual changes to CRDs and CRs are properly detected and displayed in the diff, just like they are for built-in Kubernetes resources. Fixes #917 Signed-off-by: yxxhero <aiopsclub@163.com>

Add unit tests to verify the three-way merge implementation for unstructured objects (CRDs, CRs) works correctly. These tests verify: - Manual annotations are preserved when chart doesn't change anything - Manual changes to fields are preserved when chart doesn't change them - Chart changes are applied while preserving manual changes to other fields - Chart overrides manual changes when it explicitly changes a field Related to #917 Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

This PR updates the manifest generation logic to perform a more sophisticated three-way merge for unstructured resources (CRDs/CRs) so that live state is incorporated into the patch calculation, and adds focused unit tests around this behavior.

Changes:

Modified createPatch in manifest/generate.go so that for unstructured/CRD objects it computes a merge patch by deriving chart changes from the previous release to the new manifest and then applying those changes onto the live object before generating the final patch.
Added manifest/generate_test.go with table-driven tests that exercise createPatch for several CR/CRD scenarios, verifying patch type and whether a patch is (non-)empty.
Improved error context messages in the unstructured/CRD path of createPatch.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
manifest/generate.go	Adjusts `createPatch` to use a custom three-way merge flow for unstructured/CRD objects instead of a simple old→new JSON merge patch.
manifest/generate_test.go	Introduces unit tests asserting patch type and presence/absence of diffs for various CR/CRD combinations of original, current, and target manifests.

Copilot · 2026-02-01T05:00:19Z

manifest/generate_test.go

+	tests := []struct {
+		name           string
+		original       runtime.Object
+		current        runtime.Object
+		target         *resource.Info


The table-driven tests cover several useful combinations of original/current/target, but they do not include the core scenario from issue #917 where a field that is explicitly defined in the chart remains unchanged between original and target while being modified only in the live object (e.g., spec.maxReplicaCount is 10 in both original and target, but 30 in current). Without a test for this pattern, it is easy for the unstructured/CRD three-way-merge logic to regress (or remain incorrect) while all tests here still pass. Please add a case that matches the reported bug scenario to assert that a non-empty patch is produced when live state diverges from the chart for a chart-owned field, even if the chart did not change that field between releases.

Copilot · 2026-02-01T05:00:19Z

manifest/generate_test.go

+				// No changes expected
+				if len(patch) > 0 && string(patch) != "{}" && string(patch) != "null" {
+					t.Logf("%s: got unexpected patch: %s", tt.description, string(patch))
+				}


In the expectedChange == false branch, the test only logs an unexpected non-empty patch instead of failing the test when a non-trivial patch is returned. For cases like "CR with no manual changes", an empty (or effectively empty) patch is the expected behavior, so silently accepting a non-empty patch here would hide regressions in the three-way-merge logic. Consider making this branch assert that the patch is empty (allowing only {} or "null" as appropriate) so that any future behavior change that introduces unintended diffs will cause the test to fail.

Suggested change

// No changes expected

if len(patch) > 0 && string(patch) != "{}" && string(patch) != "null" {

t.Logf("%s: got unexpected patch: %s", tt.description, string(patch))

}

// No changes expected: patch must be empty or effectively empty ("{}" or "null")

if len(patch) == 0 || string(patch) == "{}" || string(patch) == "null" {

return

}

require.Failf(t, tt.description+": expected no changes, got unexpected patch", "unexpected patch: %s", string(patch))

Copilot · 2026-02-01T05:00:20Z

manifest/generate.go

+		// Step 1: Create patch from old -> new (what the chart wants to change)
+		chartChanges, err := jsonpatch.CreateMergePatch(oldData, newData)
+		if err != nil {
+			return nil, types.MergePatchType, fmt.Errorf("creating chart changes patch: %w", err)
+		}


The three-way merge implementation for unstructured/CRD objects here will still fail to detect drift when a field is unchanged between oldData and newData but modified only in the live object (the scenario described in #917 for values explicitly set in the chart). In that case jsonpatch.CreateMergePatch(oldData, newData) returns an empty patch, mergedData ends up identical to currentData, and CreateMergePatch(currentData, mergedData) also returns an empty patch, so no diff is produced even though the live state diverges from the chart. This contradicts the PR description that manual changes to CRDs/CRs are now detected "just like" built-in resources, and it means the fix does not yet cover the core bug scenario. To align behavior with the strategic three-way merge path used for built-in types, the unstructured/CRD branch should compute a patch that also captures differences between currentData and newData for chart-owned fields, even when newData equals oldData, rather than only replaying old -> new changes onto currentData.

Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copilot · 2026-02-01T05:15:37Z

manifest/generate.go

+		// 5. If chart unchanged (old == new) but current != new, return new->current patch to restore chart state
+		//    (i.e., detect and revert manual changes to chart-owned fields)


In the step-by-step comment for the unstructured/CRD three-way merge, step 5 says "return new->current patch to restore chart state", but the implementation below calls jsonpatch.CreateMergePatch(currentData, newData), i.e. it creates a patch from current -> new. To avoid confusion for future readers, the comment should be updated to describe the same direction as the code (patch from current to new, applied to the current/live object).

Suggested change

// 5. If chart unchanged (old == new) but current != new, return new->current patch to restore chart state

// (i.e., detect and revert manual changes to chart-owned fields)

// 5. If chart unchanged (old == new) but current != new, return current->new patch applied to current to restore chart state

// (i.e., detect and revert manual changes by restoring chart-owned fields to the chart's desired state)

Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

terabyte128 · 2026-02-10T22:31:28Z

When running with this change against an Istio AuthorizationPolicy CRD, I see the following error:

  Error: unable to generate manifests: cannot patch "<<resource name>>" with kind AuthorizationPolicy: authorizationpolicies.security.istio.io "tenant-manager" is invalid: metadata.resourceVersion: Invalid value: 0: must be specified for an update

Add cleanMetadataForPatch function to remove server-managed metadata fields (resourceVersion, generation, uid, etc.) before creating merge patches. This prevents 'resourceVersion: Invalid value: 0' errors when dry-running patches against CRs/CRDs. Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Copilot · 2026-02-14T06:52:49Z

manifest/generate.go

+		// Chart didn't change (old == new), but we need to detect if current diverges
+		// from the chart state. This is the case where manual changes were made to
+		// chart-owned fields.
+		// Create a patch from current -> new to detect and restore drift
+		patch, err := jsonpatch.CreateMergePatch(cleanedCurrentData, cleanedNewData)
 		return patch, types.MergePatchType, err


In the chartChanged == false branch, CreateMergePatch(cleanedCurrentData, cleanedNewData) will generate deletions for any fields that exist only in the live object (e.g., manual annotations/labels, controllers’ injected fields, and potentially Helm’s own meta.helm.sh/* annotations if they’re not present in the rendered manifest). That makes the server dry-run “target” diverge from what a three-way merge should do and can produce noisy/incorrect diffs by removing user-owned fields when the chart didn’t change. Consider instead building a desired object by overlaying the chart state onto the live state (preserving unknown additions), then diffing current -> desired (e.g., apply a merge-patch of cleanedNewData onto cleanedCurrentData and compute a patch from current to that result).

Copilot · 2026-02-14T06:52:49Z

manifest/generate.go

+func cleanMetadataForPatch(data []byte) ([]byte, error) {
+	var objMap map[string]interface{}
+	if err := json.Unmarshal(data, &objMap); err != nil {
+		return nil, err
+	}
+
+	if metadata, ok := objMap["metadata"].(map[string]interface{}); ok {
+		delete(metadata, "resourceVersion")
+		delete(metadata, "generation")
+		delete(metadata, "creationTimestamp")
+		delete(metadata, "uid")
+		delete(metadata, "managedFields")
+		delete(metadata, "selfLink")
+	}
+
+	delete(objMap, "status")
+
+	return json.Marshal(objMap)
+}


cleanMetadataForPatch largely overlaps with deleteStatusAndTidyMetadata (manifest/util.go) but removes a different set of metadata fields/annotations (e.g., meta.helm.sh/* and deployment.kubernetes.io/revision are handled in the other helper but not here). Having two slightly different “tidy metadata” implementations risks inconsistent behavior between patch generation and manifest rendering. Suggest reusing/extracting the existing helper (or at least aligning the exact fields removed) so the same metadata is ignored everywhere.

Copilot · 2026-02-14T06:52:50Z

manifest/generate_test.go

+			expectedChange: true,
+			description:    "Manual annotation that is not in chart will cause diff to remove it (JSON merge patch semantics)",


This case asserts a non-empty patch when the only live change is an extra annotation not present in the chart. That implies the diff will attempt to remove live-only fields, which is typically not what three-way merge behavior should do (it should preserve unknown additions and only reconcile chart-owned fields). If the implementation is changed to preserve live-only fields (recommended), update this expectation to assert no effective change / that the extra annotation is preserved, and focus drift detection on chart fields (e.g. the issue #917 spec field).

Suggested change

expectedChange: true,

description: "Manual annotation that is not in chart will cause diff to remove it (JSON merge patch semantics)",

expectedChange: false,

description: "Manual annotation not present in chart should be preserved; no effective change expected from three-way merge",

…jects - Change drift detection to apply new onto current (preserves live-only fields) - Align cleanMetadataForPatch with deleteStatusAndTidyMetadata (add helm annotations) - Update test case: manual annotations should be preserved, not cause a diff This ensures three-way merge behavior matches strategic merge patch: - Live-only fields (e.g., manual annotations) are preserved - Drift on chart-owned fields (e.g., spec fields) is still detected Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Copilot · 2026-02-14T07:11:52Z

manifest/generate.go

+func cleanMetadataForPatch(data []byte) ([]byte, error) {
+	var objMap map[string]interface{}
+	if err := json.Unmarshal(data, &objMap); err != nil {
+		return nil, err
+	}
+
+	delete(objMap, "status")
+
+	if metadata, ok := objMap["metadata"].(map[string]interface{}); ok {
+		delete(metadata, "managedFields")
+		delete(metadata, "generation")
+		delete(metadata, "creationTimestamp")
+		delete(metadata, "resourceVersion")
+		delete(metadata, "uid")
+
+		// Remove helm-related and k8s annotations that shouldn't be compared
+		if a := metadata["annotations"]; a != nil {
+			annotations := a.(map[string]interface{})
+			delete(annotations, "meta.helm.sh/release-name")
+			delete(annotations, "meta.helm.sh/release-namespace")
+			delete(annotations, "deployment.kubernetes.io/revision")


cleanMetadataForPatch largely duplicates deleteStatusAndTidyMetadata (same metadata/status stripping + annotation cleanup) but now exists as a separate implementation with different JSON codec and no direct unit tests. To avoid the two clean-up paths drifting over time, consider extracting a shared helper (or reusing deleteStatusAndTidyMetadata and re-marshalling) so metadata stripping behavior is defined in one place and covered by the existing util tests.

Copilot · 2026-02-14T07:11:52Z

manifest/generate_test.go

+// TestCreatePatchForUnstructured tests the three-way merge implementation for unstructured objects (CRDs, CRs).
+// This tests the fix for issue #917 where manual changes to CRs were not being detected.
+func TestCreatePatchForUnstructured(t *testing.T) {


The new unstructured three-way-merge tests exercise the runtime.Unstructured branch, but the dedicated CRD type branch (isCRD == true) isn’t covered. Since the PR claims to fix CRDs and CRs, add at least one test case that uses an apiextensions/v1 CustomResourceDefinition object to ensure createPatch takes the CRD path and produces the expected patch behavior.

- Refactor cleanMetadataForPatch to call deleteStatusAndTidyMetadata to avoid code duplication and ensure consistent metadata handling - Add TestCreatePatchForCRD test case to cover the CRD type branch (apiextv1.CustomResourceDefinition path) Signed-off-by: yxxhero <aiopsclub@163.com>

yxxhero added 2 commits February 1, 2026 12:18

yxxhero requested a review from Copilot February 1, 2026 04:57

Copilot started reviewing on behalf of yxxhero February 1, 2026 04:57 View session

Copilot AI reviewed Feb 1, 2026

View reviewed changes

fix: properly detect drift when chart unchanged for unstructured objects

a460e71

Signed-off-by: yxxhero <aiopsclub@163.com>

yxxhero requested a review from Copilot February 1, 2026 05:13

Copilot started reviewing on behalf of yxxhero February 1, 2026 05:13 View session

Copilot AI reviewed Feb 1, 2026

View reviewed changes

fix: correct comment direction in three-way merge logic

efb7e0a

Signed-off-by: yxxhero <aiopsclub@163.com>

yxxhero requested a review from Copilot February 1, 2026 05:20

Copilot started reviewing on behalf of yxxhero February 1, 2026 05:21 View session

Copilot AI reviewed Feb 1, 2026

View reviewed changes

yxxhero requested a review from Copilot February 14, 2026 06:46

Copilot started reviewing on behalf of yxxhero February 14, 2026 06:47 View session

Copilot AI reviewed Feb 14, 2026

View reviewed changes

yxxhero requested a review from Copilot February 14, 2026 07:05

Copilot started reviewing on behalf of yxxhero February 14, 2026 07:06 View session

Copilot AI reviewed Feb 14, 2026

View reviewed changes

		// 5. If chart unchanged (old == new) but current != new, return new->current patch to restore chart state
		// (i.e., detect and revert manual changes to chart-owned fields)

		expectedChange: true,
		description: "Manual annotation that is not in chart will cause diff to remove it (JSON merge patch semantics)",

Conversation

yxxhero commented Feb 1, 2026

Summary

Changes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

terabyte128 commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants